Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support new GitHub bot deployment #15

Merged
merged 24 commits into from
Jun 25, 2024
Merged

Support new GitHub bot deployment #15

merged 24 commits into from
Jun 25, 2024

Conversation

ChengaDev
Copy link
Member

@ChengaDev ChengaDev commented May 1, 2024

  • Enable deployment of our new GitHub bot via Terraform
  • Enable several instances of the bot in a single region
  • Enable set custom name pattern for all module resources
  • Allow the user to set a path to the required source code files he wants to deploy to the lambdas
  • Update runtime to nodejs20.x

@ChengaDev ChengaDev added the wip Work in progress label May 1, 2024
@@ -152,3 +152,7 @@ module "spectral_lambda_integration" {
8. `lambda_iam_role_arn` - Amazon Resource Name (ARN) specifying the role.
9. `lambda_iam_role_name` - Name of the role.
10. `secrets_arns` - Arns of created secrets in secrets manager.

## Support
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a changelog file that lists versions, describes the changes, and breaks changes per version is essential, and can avoid a lot of questions.

@@ -14,14 +14,14 @@ Terraform configuration used to create the required AWS resources for integratin

| Name | Version |
| ----------- | ----------- |
| [aws](https://registry.terraform.io/providers/hashicorp/aws/latest/docs) | >= 4.37.0 |
| [aws](https://registry.terraform.io/providers/hashicorp/aws/latest/docs) | >= 5.26.0 |

## Inputs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some missing fields in the documentation. check out this tool https://terraform-docs.io/ it can help you to manage it
lambda_source_code_path
frontend_lambda_source_code_path
backend_lambda_source_code_path
lambda_logs_retention_in_days
lambda_enable_logs
resource_name_common_part
secrets_names
gateway_api_integration_timeout_milliseconds

@@ -30,7 +30,7 @@ Terraform configuration used to create the required AWS resources for integratin
| `lambda_function_timeout` | Amount of time your Lambda Function has to run in seconds. | `number` | 300 | No |
| `lambda_function_memory_size` | Amount of memory in MB your Lambda Function can use at runtime. | `number` | 1024 | No |
| `lambda_publish` | Whether to publish creation/change as new Lambda Function Version. | `bool` | `false` | No |
| `store_secret_in_secrets_manager` | Whether to store secrets values on a vault (currently supporting AWS secrets manager on GitLab integration). | `bool` | `false` | No |
| `store_secret_in_secrets_manager` | Whether to store secrets values on a vault (currently supporting AWS secrets manager on GitHub / GitLab integration). | `bool` | `false` | No |

### env_vars
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section is partially relevant to the bots.
i think we can remove this section and redirect to our guide docs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The env vars are not detailed in this section, this just shows how to set variables and redirects the users to the docs.
The only env var mentioned here is SPECTRAL_DSN which is always mandatory.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the var that is mandatory, in a different but, there is more mandatory env var. i just say to refer to our docs and remote this section

api_triggered_function_arn = local.single_lambda_integration ? module.lambda_function[0].lambda_function_arn : module.frontend_lambda_function[0].lambda_function_arn
frontend_lambda_handler = contains(["github"], var.integration_type) ? "index.handler" : "frontend.app"
backend_lambda_handler = contains(["github"], var.integration_type) ? "index.handler" : "backend.app"
default_secrets_names = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about SPECTRAL_DSN?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct

locals.tf Outdated
"gitlab" = coalesce(var.secrets_names, ["Spectral_GitlabBot_GitlabToken", "Spectral_GitlabBot_WebhookSecret"])
}
# Please do not change or replace the 'frontend' suffix since there a logic in the bot based in it
function_name = local.single_lambda_integration ? local.resource_name_pattern : "${local.resource_name_pattern}-frontend"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function_name can be confusing since it implies working with both the backend and the front end. I recommend the following changes:

Rename options:

  • receive_hook_lambda_name
  • provider_request_treigger_lambda_name

The idea is to understand when you read this variable what he does.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to api_integrated_function_name

variable "lambda_source_code_filename" {
type = string
description = "The lambda source code filename"
}

variable "lambda_source_code_path" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use terraform-docs to generate the readme file for the lambda module.
this is relevant for all the modules

environment = var.environment
integration_type = var.integration_type
# Please do not change or replace the 'frontend' suffix since there a logic in the bot based in it
resource_name_pattern = "${local.resource_name_pattern}-frontend"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently have ${local.resource_name_pattern}-frontend in the locals.

Can we use the same variable to reduce the string concatenations?

shared.tf Outdated
}

module "lambda_role" {
source = "./modules/role"
resource_name_pattern = local.single_lambda_integration ? local.resource_name_pattern : "${local.resource_name_pattern}-frontend"
role_name = local.function_name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the role name getting the function name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this role and its policies were created explicitly for this lambda function, and with this function name there will be no doubt about why this role was created and who should use it

@ChengaDev ChengaDev added readyforreview and removed wip Work in progress labels Jun 3, 2024
@ChengaDev ChengaDev changed the title WIP - Support new GitHub bot deployment Support new GitHub bot deployment Jun 3, 2024
@ChengaDev ChengaDev merged commit 6e71c1a into main Jun 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants